Skip to content

Added edge case behavior testing for math bruteforce#2665

Open
shajder wants to merge 7 commits intoKhronosGroup:mainfrom
shajder:add_math_brute_force_edge_cases
Open

Added edge case behavior testing for math bruteforce#2665
shajder wants to merge 7 commits intoKhronosGroup:mainfrom
shajder:add_math_brute_force_edge_cases

Conversation

@shajder
Copy link
Copy Markdown
Contributor

@shajder shajder commented Apr 16, 2026

Fixes #2641

Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated

static const AbstractEdgeCase edge_case_table[] = {

{ "acospi", { ONE }, POS_ZERO },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table only covers the cases listed in the OpenCL C specification as fas as I can tell? Should we also check for the cases from Section F.9 of the C99 Specification? I'd suggest keeping the OpenCL and C99 lists somehow distinct (i.e., not interleaved).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly we should support half_ versions. As for the rest I would appreciate WG expertise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moreover, this discussion may be relevant

Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated

{
std::array<uint8_t, CL_VALUE_MAX_BYTES> result{};
static std::vector<uint8_t> result;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why static?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid continuous allocations/deallocations, the vector retains its capacity between calls so resize() does not reallocate as long as the size stays within the previously allocated capacity.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The potential benefit here looks marginal in the bigger scheme of things, and I don’t think it outweighs the readability/maintainability cost of introducing hidden persistent state. So I would encourage dropping static.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, working buffers turned into class members.

Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
err =
clEnqueueWriteBuffer(queue, buf, CL_TRUE, 0, ec.inputs[i].byte_size,
ec.inputs[i].data.data(), 0, nullptr, nullptr);
static std::vector<uint8_t> inData;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why static?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread test_conformance/math_brute_force/edge_cases.cpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

math bruteforce is not testing edge case behavior

3 participants